-
Notifications
You must be signed in to change notification settings - Fork 18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added spam and troll detection #152
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
really cool stuff, left some comments about code style
src/components/messageListener.ts
Outdated
// Soft bans member and deletes messages from past 1 day | ||
try { | ||
if (await message.member?.ban({ days: 1, reason: 'Spammer/troll/got hacked' })) { | ||
await message.guild?.members.unban(message.author.id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, is there a reason why we need to wrap this in an if statement, if ban
fails, wouldn't it throw an error and skip unban
? I.e. is there a case where ban
returns false
instead of throwing an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the if condition, message.member
might be null, in which undefined would returned, which would evaluate to false. I guess in this case though, message.member
would never be null based on the if statement above it, but I guess this might change in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you test out a code snippet that would trigger spam detection? like @everyone give nitro
Also, plz ban for 1 days and 1 hour in case someone auto spams exactly and every 24 hours.
Do we have protection against an exec or mod or advisor or someone of somewhat higher CSC status accidently getting banned and all msgs deleted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, the inline code block also triggers spam detection.
Done.
The Executive, Discord Mod, and Advisor roles are above the Codey role, so Codey can't possibly ban anyone with these roles.
…s to db.ts to remove circular dependencies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also constants -> config if not done already :)
Rest looks good to me - ty Charles u beat me to it.
src/components/messageListener.ts
Outdated
// Soft bans member and deletes messages from past 1 day | ||
try { | ||
if (await message.member?.ban({ days: 1, reason: 'Spammer/troll/got hacked' })) { | ||
await message.guild?.members.unban(message.author.id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you test out a code snippet that would trigger spam detection? like @everyone give nitro
Also, plz ban for 1 days and 1 hour in case someone auto spams exactly and every 24 hours.
Do we have protection against an exec or mod or advisor or someone of somewhat higher CSC status accidently getting banned and all msgs deleted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there protection for general PC members?
would be good cause not everyone is exec, mod, or advisor
src/components/messageListener.ts
Outdated
// Temp ban member for 1 day and 1 hour, and delete messages from past 1 day | ||
try { | ||
if (await message.member?.ban({ days: 1, reason: 'Spammer/troll/got hacked' })) { | ||
setTimeout(async () => await message.guild?.members.unban(message.author.id), 25 * 60 * 60 * 1000); // 1 day and 1 hour in milliseconds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implementation may not be resilient. Let's say if a user gets banned, and then the bot crashes or a new version gets deployed within 24 hours, then the user will not get unbanned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alternative solution that I can think of is that the time of unban would be stored in the database and there would be a cron job to check if it is time to do any unbans? Also, is the current implementation okay or is a more resilient implementation wanted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A db isn't needed as long as we print out exactly who was banned in # moderation and when they were banned, prior to actually kicking them.
We can manually do unbans upon request, which is probably very rare anyways.
What if we KICKED instead of banning for some amt of time? Right now, I think msgs disappear, which might not always be good. Suppose I stopped being a mod, and became a norm member, and got hacked, and made spam, hence banned - then would my 4000+ msgs all disappear?
Yup, similarly, the PC role is above the Codey role, so Codey can't ban people with the PC role. I guess in the case that the roles get moved around, then the PC category check would help. For the official documentation, according to https://discord.com/developers/docs/topics/permissions, "A bot can only kick, ban, and edit nicknames for users whose highest role is lower than the bot's highest role." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 minor comment otherwise LGTM
src/components/messageListener.ts
Outdated
// Delete the message, and kick the member if they are still in the server | ||
try { | ||
await message.delete(); | ||
await message.member?.kick('Spammer/troll/got hacked'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we want to send the mod channel some kind of message? so we know someone was kicked
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
A db isn't needed as long as we print out exactly who was kicked in # moderation and the msg they were kicked for, and when they were banned, prior to actually kicking them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also :codey-poggers: to @Picowchew for getting this done so fast!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup done, except that I did the logging after the kick, since I don't think we want there to be logging if the kick doesn't go through (e.g. Codey cannot kick a mod).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great stuff! :D
wait but that's optional ig |
a770b5c
to
4651441
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome :) i guess u can merge and tell ppl to pull latest since there are some changes that effect a lot of files
Related Issues
Resolves #138
Summary of Changes
If someone does @ everyone or @ here and includes a link with "http" or "nitro" outside of the PC category or if someone sends a message in the honeypot channel, then they get kicked and their relevant messages get deleted. The kick gets logged.
Steps to Reproduce
Try the above